fix(dashboard): update scrollbar contrast on background - #62244
Conversation
joshtrichards
added
bug
3. to review
joshtrichards
changed the title
| #content { | ||
| overflow: auto; | ||
| /* Scrollbar sits on the background image — use plain-text color for contrast */ | ||
| scrollbar-color: var(--color-background-plain-text) transparent; |
There was a problem hiding this comment.
I'm not sure this is versatile enough for all possible backgrounds: e.g. high-contrast themes, custom background images, or maintaining contrast along the entire scrollbar edge of an arbitrary image. Part of the approach used for the dashboard #header might be applicable/re-usable.
There was a problem hiding this comment.
@joshtrichards Thank you for the comment.
Dashboard Header always uses --color-background-plain-text which I used for the scrollbar.
Also I tried with different scenarios, custom background images and colors and high-contrast themes, it works as expected.
There was a problem hiding this comment.
I'm not sure this is versatile enough for all possible backgrounds: e.g. high-contrast themes, custom background images, or maintaining contrast along the entire scrollbar edge of an arbitrary image. Part of the approach used for the dashboard #header might be applicable/re-usable.
It is, that color is calculated from the background image to have the maximum contrast (its the icons color).
There was a problem hiding this comment.
Ah, got it - that makes sense. Reusing the header foreground color is probably the best approach here. Trying to derive a separate color specifically for a vertical scrollbar would get messy, and maintaining contrast along the entire scrollbar edge of an arbitrary image is out of scope. This seems good enough to me.
|
(tested and works as expected) |
|
/backport to stable34 |
|
/backport to stable33 |
| #content { | ||
| overflow: auto; | ||
| /* Scrollbar sits on the background image — use plain-text color for contrast */ | ||
| scrollbar-color: var(--color-background-plain-text) transparent; |
There was a problem hiding this comment.
Ah, got it - that makes sense. Reusing the header foreground color is probably the best approach here. Trying to derive a separate color specifically for a vertical scrollbar would get messy, and maintaining contrast along the entire scrollbar edge of an arbitrary image is out of scope. This seems good enough to me.
joshtrichards
added
4. to release
ff88c7b to
a0759c4
Compare
a0759c4 to
6084a5e
Compare
Signed-off-by: root <root@ip-208-84-103-161.my-advin.com>
6084a5e to
af2a382
Compare
|
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
3 participants
Summary
Checklist
3. to review, feature component)stable32)AI (if applicable)